Add Stream wrappers for memory and text-based types#126669
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces new standardized Stream wrapper types across CoreLib and System.Memory, aligning with the approved API shape from #82801 and updating a few internal call sites to use the new wrappers.
Changes:
- Added new public sealed stream wrappers:
StringStream,ReadOnlyMemoryStream,WritableMemoryStream(CoreLib) andReadOnlySequenceStream(System.Memory). - Updated reference assemblies and project files to expose/compile the new APIs.
- Added conformance + targeted behavioral tests, and updated select internal consumers to use
StringStream.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/IO/StringStream.cs | Implements the new StringStream API (read-only, non-seekable, on-the-fly encoding). |
| src/libraries/System.Private.CoreLib/src/System/IO/ReadOnlyMemoryStream.cs | Implements seekable read-only stream over ReadOnlyMemory<byte>. |
| src/libraries/System.Private.CoreLib/src/System/IO/WritableMemoryStream.cs | Implements seekable fixed-capacity read/write stream over Memory<byte>. |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Wires new CoreLib stream wrapper source files into the build. |
| src/libraries/System.Private.CoreLib/src/Resources/Strings.resx | Adds a new resource string entry (currently appears unused). |
| src/libraries/System.Runtime/ref/System.Runtime.cs | Updates public API surface to include the new stream wrapper types (and a small ref signature normalization change). |
| src/libraries/System.Runtime/tests/System.IO.Tests/System.IO.Tests.csproj | Includes the new test files in the System.IO test project. |
| src/libraries/System.Runtime/tests/System.IO.Tests/StringStream/StringStreamConformanceTests.cs | Adds conformance coverage for StringStream (string + ReadOnlyMemory<char> overloads). |
| src/libraries/System.Runtime/tests/System.IO.Tests/StringStream/StringStreamTests_String.cs | Adds targeted StringStream(string, Encoding) behavioral tests. |
| src/libraries/System.Runtime/tests/System.IO.Tests/StringStream/StringStreamTests_Memory.cs | Adds targeted StringStream(ReadOnlyMemory<char>, Encoding) behavioral tests. |
| src/libraries/System.Runtime/tests/System.IO.Tests/ReadOnlyMemoryStream/ReadOnlyMemoryStreamConformanceTests.cs | Adds conformance coverage for ReadOnlyMemoryStream. |
| src/libraries/System.Runtime/tests/System.IO.Tests/ReadOnlyMemoryStream/ReadOnlyMemoryStreamTests.cs | Adds targeted ReadOnlyMemoryStream behavioral tests. |
| src/libraries/System.Runtime/tests/System.IO.Tests/WritableMemoryStream/WritableMemoryStreamConformanceTests.cs | Adds conformance coverage for WritableMemoryStream (with some overridden tests). |
| src/libraries/System.Runtime/tests/System.IO.Tests/WritableMemoryStream/WritableMemoryStreamTests.cs | Adds targeted WritableMemoryStream behavioral tests. |
| src/libraries/System.Memory/src/System/Buffers/ReadOnlySequenceStream.cs | Implements ReadOnlySequenceStream over ReadOnlySequence<byte>. |
| src/libraries/System.Memory/src/System.Memory.csproj | Includes the new ReadOnlySequenceStream source file in System.Memory. |
| src/libraries/System.Memory/src/Resources/Strings.resx | Adds SR strings needed for stream-like exception messages in System.Memory. |
| src/libraries/System.Memory/ref/System.Memory.cs | Adds ReadOnlySequenceStream to the System.Memory ref surface. |
| src/libraries/System.Memory/tests/System.Memory.Tests.csproj | Adds tests for ReadOnlySequenceStream and references StreamConformanceTests. |
| src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceStreamTests.cs | Adds targeted behavioral tests for multi-segment ReadOnlySequenceStream scenarios. |
| src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceStream.ConformanceTests.cs | Adds conformance coverage for ReadOnlySequenceStream. |
| src/libraries/System.Private.Xml/src/System/Xml/Resolvers/XmlPreloadedResolver.cs | Switches internal string-to-stream conversion to StringStream. |
| src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/Json/JsonXmlDataContract.cs | Switches internal string-to-stream conversion to StringStream. |
Comments suppressed due to low confidence (1)
src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/Json/JsonXmlDataContract.cs:38
- Variable is still named 'memoryStream' but the implementation is now StringStream (not a MemoryStream). Rename the local to avoid implying the stream is seekable/in-memory-buffered (e.g., 'xmlContentStream').
Stream memoryStream = new StringStream(xmlContent, Encoding.UTF8);
object? xmlValue;
XmlDictionaryReaderQuotas? quotas = ((JsonReaderDelegator)jsonReader).ReaderQuotas;
if (quotas == null)
{
xmlValue = dataContractSerializer.ReadObject(memoryStream);
27761ff to
b43698b
Compare
b43698b to
95a2989
Compare
95a2989 to
5e3f98c
Compare
…onvert edge case. Flush strategy update.
5e3f98c to
fec7a59
Compare
adamsitnik
left a comment
There was a problem hiding this comment.
I really like the simplicity of the design and implementation.
Overall it LGTM, I found mostly some nits related to tests. I will need to perform another round for StringStream.
Thank you @ViveliDuCh !
|
Note AI-generated content (Copilot) Base Class Benchmark:
|
|
Note AI-generated content (Copilot) Base Class Benchmark:
|
…s, and conformance coverage
d8e4b7a to
8e4a441
Compare
| public class ReadOnlySequenceStreamTests | ||
| { | ||
| [Fact] | ||
| public void ReadZeroBytesReturnsZero() |
There was a problem hiding this comment.
This seems like a duplicate of
There was a problem hiding this comment.
Can you try to ask copilot to cross-reference all your tests, so they are not duplicating the ones from StreamConformanceTests. That's how I found these.
| } | ||
|
|
||
| [Fact] | ||
| public void SeekingBeyondEmptyBufferIsAllowed() |
There was a problem hiding this comment.
We should maybe instead update Seek_PastEnd_ReadReturns0 to assert Position is updated when seeking past stream.
There was a problem hiding this comment.
Decided to keep the Position assertion local rather than push into Seek_PastEnd_ReadReturns0 — the conformance test already asserts Seek returns pos, which by the Stream.Seek contract equals Position, so adding Assert.Equal(pos, stream.Position) to the shared test would be redundant and risks failing any stream whose Position impl quietly diverges from its Seek return value (e.g. buffered streams when seeking past end). The narrow assertion in this PR's tests is enough to guard the wrapper's behavior without changing the contract surface for every other stream in the repo.
| Task<int> task1 = stream.ReadAsync(buffer1, 0, 5); | ||
| Task<int> task2 = stream.ReadAsync(buffer2, 0, 5); | ||
| Task<int> task3 = stream.ReadAsync(buffer3, 0, 5); | ||
|
|
||
| await task1; | ||
| await task2; | ||
| await task3; | ||
|
|
||
| Assert.Same(task1, task2); | ||
| Assert.Same(task2, task3); | ||
|
|
There was a problem hiding this comment.
You may want to reconsider this one, I don't see the Task caching optimization built into ReadOnlySequenceStream.
| } | ||
|
|
||
| [Fact] | ||
| public async Task ReadAsyncArrayBackedMemoryUsesFastPath() |
There was a problem hiding this comment.
This isn't testing any fast path I believe and there's nothing special about it. I suggest removing it.
| { | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
Have you considered adding a single-shot Encoding.GetBytes fast path? It is ~34% faster than using Encoder.Convert and avoids some allocations: https://gist.github.com/jozkee/ec72a16575edac7fcbde864cde61cd76.
Results:
| Method | Length | Mean | Error | StdDev | Ratio | Gen0 | Allocated | Alloc Ratio |
|---|---|---|---|---|---|---|---|---|
| Baseline | 16 | 40.10 ns | 6.150 ns | 0.337 ns | 1.00 | 0.0100 | 168 B | 1.00 |
| OneShot | 16 | 26.60 ns | 2.302 ns | 0.126 ns | 0.66 | 0.0076 | 128 B | 0.76 |
| Baseline | 1024 | 57.02 ns | 3.955 ns | 0.217 ns | 1.00 | 0.0100 | 168 B | 1.00 |
| OneShot | 1024 | 42.24 ns | 8.008 ns | 0.439 ns | 0.74 | 0.0076 | 128 B | 0.76 |
| // cannot hold a single complete encoded character. | ||
| if (buffer.Length < _encoding.GetMaxByteCount(1)) | ||
| { | ||
| _pendingBytes ??= new byte[_encoding.GetMaxByteCount(2)]; |
There was a problem hiding this comment.
This could benefit a bit from GC.AllocateUninitializedArray, as done in
|
|
||
| _text = text.AsMemory(); | ||
| _encoding = encoding; | ||
| _encoder = encoding.GetEncoder(); |
There was a problem hiding this comment.
If we do #126669 (comment), we should lazy-init this.
| _disposed = true; | ||
| base.Dispose(disposing); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should CopyTo[Async] be overridden?
…ridging, Seek hardening, and test dedup
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime/tests/System.IO.Tests/WritableMemoryStream/WritableMemoryStreamTests.cs:118
- The new WritableMemoryStream type documents that GetBuffer throws and TryGetBuffer returns false, but the tests here don't assert that behavior. Adding a small test would lock in the intended contract (and distinguish it from MemoryStream's default behavior).
Assert.Equal(data, manager.GetSpan().ToArray());
Assert.Equal(data.Length, stream.Position);
}
}
}
| if (_charPosition == 0 && _pendingCount == 0 && | ||
| buffer.Length >= _encoding.GetMaxByteCount(_text.Length)) | ||
| { | ||
| int written = _encoding.GetBytes(_text.Span, buffer); | ||
| _charPosition = _text.Length; | ||
| _encoderFlushed = true; | ||
| return written; | ||
| } |
| // Size the rented buffer to the remaining encoded payload (capped by bufferSize). | ||
| // The base Stream.CopyTo falls back to an 80 KB buffer because our Length throws; | ||
| // sizing here avoids that waste and lets the single-shot Read fast path consume | ||
| // the entire input in one call when the rented buffer is large enough. | ||
| int maxBytes = _encoding.GetMaxByteCount(_text.Length - _charPosition); | ||
| int rentSize = Math.Max(1, Math.Min(maxBytes, bufferSize)); | ||
| byte[] buffer = ArrayPool<byte>.Shared.Rent(rentSize); |
| int maxBytes = _encoding.GetMaxByteCount(_text.Length - _charPosition); | ||
| int rentSize = Math.Max(1, Math.Min(maxBytes, bufferSize)); | ||
| return CopyToAsyncCore(destination, rentSize, cancellationToken); |
| long newPosition = origin switch | ||
| { | ||
| SeekOrigin.Begin => offset, | ||
| SeekOrigin.Current => _position + offset, | ||
| SeekOrigin.End => _buffer.Length + offset, | ||
| _ => throw new ArgumentOutOfRangeException(nameof(origin)) | ||
| }; |
| long newPosition = origin switch | ||
| { | ||
| SeekOrigin.Begin => offset, | ||
| SeekOrigin.Current => _position + offset, | ||
| SeekOrigin.End => _length + offset, | ||
| _ => throw new ArgumentOutOfRangeException(nameof(origin)) | ||
| }; |
| long absolutePosition = origin switch | ||
| { | ||
| SeekOrigin.Begin => offset, | ||
| SeekOrigin.Current => _absolutePosition + offset, | ||
| SeekOrigin.End => _sequence.Length + offset, | ||
| _ => throw new ArgumentOutOfRangeException(nameof(origin)) | ||
| }; |
|
|
||
| public sealed class ReadOnlySequenceStream : Stream | ||
| { |
| } | ||
| } | ||
| } |
…ds, GetBuffer contract tests, ReadOnlySequenceStream XML doc
Fixes #82801
Introduces standardized stream wrappers for text and memory types as public sealed classes (
StringStream,ReadOnlyMemoryStream,WritableMemoryStream, andReadOnlySequenceStream) as approved in API review (video).What's included
StringStream(System.IO, CoreLib): non-seekable, read-only stream that encodesstringorReadOnlyMemory<char>on-the-fly. Encoding is required (no default). Never emits BOMs.ReadOnlyMemoryStream(System.IO, CoreLib): seekable, read-only stream overReadOnlyMemory<byte>WritableMemoryStream(System.IO, CoreLib): seekable, read-write stream overMemory<byte>with fixed capacityReadOnlySequenceStream(System.Buffers, System.Memory): seekable, read-only stream overReadOnlySequence<byte>System.RuntimeandSystem.MemoryReadOnlyMemoryContent,XmlPreloadedResolver,JsonXmlDataContract) to use the new typesImplementation notes
public sealedwith direct constructors: the review moved away from factory methods onStreamto avoid derived-type IntelliSense noiseStringStreamencodes directly into the caller's buffer inRead()rather than using an internal byte bufferReadOnlyMemoryStreamandWritableMemoryStreamderive fromMemoryStream.TryGetBufferreturns false andGetBufferthrows on the memory stream types (nopubliclyVisiblesupport for now)Limitations
ReadOnlyMemoryStreaminCommon/cannot be fully replaced:System.Memory.Datamulti-targetsnetstandard2.0where the new public type doesn't exist.System.Net.Http(net11.0 only) can adopt it directly.Follow-up work
IBufferWriter<byte>→Streamwrapper (separate proposal per #100434)